PARQUET-2415: Reuse hadoop file status and footer in ParquetRecordReader #1242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wankunde
force-pushed
the
footer_and_rpc
branch
from
6ee8649
to
10d74ed
Compare
wankunde
force-pushed
the
footer_and_rpc
branch
from
10d74ed
to
0321a2d
Compare
|
I can not reproduce the failed UT: If any one could help check this issue ? |
|
Have you tried to run mvn install before running the cli test? It may run the test with dependency from maven central without your patch. |
wankunde
force-pushed
the
footer_and_rpc
branch
from
0a9c353
to
8bca57b
Compare
|
Hi, @wgtmac I have fixed this issue in cli module. Thanks |
wgtmac
reviewed
Dec 25, 2023
| @@ -84,6 +86,9 @@ public static ParquetMetadata fromJSON(String json) { | |||
| private final FileMetaData fileMetaData; | |||
| private final List<BlockMetaData> blocks; | |||
|
|
|||
| @JsonIgnore | |||
There was a problem hiding this comment.
Why is this annotation required?
There was a problem hiding this comment.
The jackson mapper will not serialize this field to json with this annotation and keep the the same behavior as before.
wgtmac
reviewed
Dec 25, 2023
| HadoopInputFile inputFile; | ||
| if (split.getFooter() != null | ||
| && split.getFooter().getInputFile() != null | ||
| && split.getFooter().getInputFile() instanceof HadoopInputFile) { |
There was a problem hiding this comment.
cc @amousavigourabi to see if there is any chance to apply this to the non-hadoop code path
There was a problem hiding this comment.
Hi @wgtmac, given that the inputFile variable seems to only be used in a constructor expecting an InputFile and not necessarily a HadoopInputFile, I think this instanceof condition could be dropped. As I just quickly skimmed it now and might have missed something, I'll take a more thorough look after Boxing Day. Happy holidays!🎄🎆
There was a problem hiding this comment.
Thanks @wgtmac @amousavigourabi for your review.
I have changed the HadoopInputFile to InputFile
There was a problem hiding this comment.
Looking good! Going back to @wgtmac's earlier concern, the method this snippet is part of is already in the Hadoop code path and I'm not sure whether there is a more generic alternative available. For the rest, the switch to using the plain InputFile interface here is of course amazing for flexibility in the future and makes the code a bit cleaner. Thanks a lot for the swift fix @wankunde!
|
Hi, @wgtmac @amousavigourabi is there any concern about this PR ? |
wgtmac
approved these changes
Jan 13, 2024
There was a problem hiding this comment.
+1 on my side.
I think this should be a very common requirement and not sure if the community has discussed this before. cc @gszadovszky @shangxinli @Fokko @ConeyLiu
|
Hi @wankunde , sorry for the delayed response. I don't see any blockers on my side and love the patch, so its a +1 from me. |
amousavigourabi
approved these changes
Jan 14, 2024
shangxinli
reviewed
Jan 15, 2024
| @@ -95,6 +95,11 @@ | |||
| <artifactId>jackson-databind</artifactId> | |||
| <version>${jackson-databind.version}</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>${jackson.groupId}</groupId> | |||
There was a problem hiding this comment.
Can we avoid adding dependency?
There was a problem hiding this comment.
The jackson-annotations dependency is used in parquet-hadoop/src/main/java/org/apache/parquet/hadoop/metadata/ParquetMetadata.java . Do not serialize InputFile inputFile to json and keep the the same behavior as before. I'm sorry I'm not familiar with jackson library and not sure is there any other way to do this.
There was a problem hiding this comment.
I happened to find that we have a parquet-jackson module which shades jackson-core and jackson-databind. But in the parquet-hadoop (and other modules) it also explicitly depends on parquet-jackson and jackson-xxx at the same time. I'm not familiar with this history, do you know why? @gszadovszky @Fokko @shangxinli
ConeyLiu
reviewed
Jan 15, 2024
| return footer; | ||
| } | ||
|
|
||
| public void setFooter(ParquetMetadata footer) { |
There was a problem hiding this comment.
The ParquetInputSplit is marked as deprecated. And the recommended usage is FileSplit. How does Spark set the footer after the ParquetInputSplit is removed?
There was a problem hiding this comment.
Now parquet will convert the input split to ParquetInputSplit and build the reader with it. I think if ParquetInputSplit was removed from ParquetFileReader class, spark need a shim class to work with different parquet version.
That will be a big change.
There was a problem hiding this comment.
I think this PR can be a good reason to push the spark community to migrate. Or we can fix this in only spark 4.x?
There was a problem hiding this comment.
Already filed a WIP ticket apache/spark#44853 for spark 4 and will discuss about this change in spark side in that PR after this PR is merged.
There was a problem hiding this comment.
IIUC, other comments have suggested that we should not work on a deprecated interface. Therefore I don't expect this PR will be merged as is. It would be good to figure out the final solution on the spark side before any action here.
ConeyLiu
reviewed
Jan 15, 2024
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/metadata/ParquetMetadata.java
Show resolved
Hide resolved
ConeyLiu
reviewed
Jan 15, 2024
| @@ -64,13 +65,19 @@ public int run() throws IOException { | |||
| return 0; | |||
| } | |||
|
|
|||
| abstract class MixIn { | |||
| @JsonIgnore | |||
| abstract int getInputFile(); | |||
There was a problem hiding this comment.
Is there any better way to do this? What about annotating the getInputFile at https://github.com/apache/parquet-mr/pull/1242/files#diff-69ea2eba95ed65129e4a4a9e5807807a3c138ea39f2eddf5d6f6b8f2a3b51c73R115?
There was a problem hiding this comment.
I'm sorry, the UT failed, I don't know why.
There was a problem hiding this comment.
You mean this is a workaround to get rid of the test failure at the cost of a new dependency?
There was a problem hiding this comment.
I mean the UT will fail if just annotating the getInputFile method, create a MixIn class here (parquet-cli module) to workaround.
Parquet project already has a dependency of jackson-annotations library in some other modules. So I don't think this PR will add a new dependency in parquet-hadoop module.
|
+1 for the concept. We need to address that |
| if (split.getFooter() != null && split.getFooter().getInputFile() != null) { | ||
| inputFile = split.getFooter().getInputFile(); | ||
| } else { | ||
| inputFile = HadoopInputFile.fromPath(path, configuration); |
There was a problem hiding this comment.
if the filestatus (or at least file length) can get down here then it becomes possible to skip a HEAD request when opening a file against cloud storage. the api you need is in 3.3.0, and not very reflection friendly. we could add something to assist there.
what is key is: get as much info as possible into HadoopInputFile, especially expected length
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Make sure you have checked all steps below.
Jira
them in the PR title. For example, "PARQUET-2415: Reuse hadoop file status and footer in ParquetRecordReader"
Tests
Commits
from "How to write a good git commit message":
Style
mvn spotless:apply -Pvector-pluginsDocumentation
ParquetInputSplit.setFooter(footer)before creating a ParquetRecordReader from this split, then the reader will reuse the Hadoop file status and skip the getfileinfo Hadoop RPC and skip reading the footer again.